Skip to content

Fix: AttemptBuilder.buildAttempts silently skips invalid model strings (TODO flagged in source) - user typos in fallback lists produce no clear error - #5836

Open
SIDDARTHAREDDY8 wants to merge 1 commit into
Helicone:mainfrom
SIDDARTHAREDDY8:oss-bhai-2026-10-12
Open

SIDDARTHAREDDY8 wants to merge 1 commit into
Helicone:mainfrom
SIDDARTHAREDDY8:oss-bhai-2026-10-12

Conversation

@SIDDARTHAREDDY8

Copy link
Copy Markdown

Fixes #5759.

Implemented the fix for #5759 on branch oss-bhai-2026-10-12 (fork SIDDARTHAREDDY8/helicone, commit cbb3eb6, base main, verified from the API). AttemptBuilder.buildAttempts now returns Promise<Result<Attempt[], string>>: on isErr(modelSpec) it keeps the console.error (renamed to 'Invalid model string') and returns err(modelSpec.error) instead of silently continuing; the maintainer's '// TODO: Return error' comment is removed; both final returns wrap in ok(...). The sole caller (verified: only SimpleAIGateway.ts:176 calls buildAttempts; code search total 3 files incl. docs) now checks isErr(attemptsResult) before the empty-array path and returns createErrorResponse with {source: 'Invalid model', message: attemptsResult.error, statusCode: 400}. ONE deliberate deviation from the issue's literal code sketch, verified by driving the real createErrorResponse: the issue sketched type 'request_failed', but the priority chain in createErrorResponse routes a 'request_failed' 400 into the firstNon429Error branch and normalizes it to HTTP 500 (confirmed by harness: request_failed+400 -> 500). The pushed error therefore uses type 'invalid_format', the only type the chain maps to HTTP 400, so the acceptance criterion 'response is a 400 naming the bad model id' actually holds at runtime. COLLISION CHECK NOTE: the step-0 check found PR #5760 ('Fixes #5759') by Harsh23Kashyap, but it is CLOSED and NOT merged (merged_at null, closed_by null = author-closed on 2026-08-18, zero maintainer reviews/comments; only a vercel bot comment). Per the check's literal stop condition it blocks only on OPEN or MERGED PRs, so the build proceeded; issue #5759 is still open and unfixed on main (TODO and silent-skip confirmed present in the fetched main source). The reviewer may still weigh #5760's history; it is flagged here rather than hidden.

Tests pass: Repo vitest suite NOT run locally: it needs the full monorepo install (node_modules + @cloudflare/vitest-pool-workers/miniflare), which the lean API workdir does not have. Instead, a focused harness (in /tmp/harness-build) transpiled the REAL edited AttemptBuilder.ts and SimpleAIGateway.ts plus the REAL parseModelString/validateProvider (packages/cost/models/provider-helpers.ts) and the REAL results modules, stubbing only heavy imports (registry data, provider keys, tracer). 21/21 checks passed: (1) valid model strings return ok wrapping the attempt array, exactly as before; (2) an invalid model string returns err naming the offending string ('Unknown model: gpt-4o-min...'); (3) mixed lists fail on the first invalid entry in both orders; (4) an invalid provider returns err naming the provider; (5) the REAL createErrorResponse priority chain maps the pushed error to HTTP 400 carrying the parser message, with details[0].source 'Invalid model'. Both edited files and the new spec transpile with zero TypeScript diagnostics. The new spec file (5 builder unit tests + 2 end-to-end gateway tests via the repo's own runGatewayTest asserting finalStatus 400 + responseContains 'Unknown model: gpt-4o-min') is written for the repo's own vitest runner; it could not be executed here.

Ticket

Fixes #5759

Component/Service

  • AI Gateway

Type of Change

  • Bug fix

Deployment Notes

  • No special deployment steps required

Context

See description above.

…ildAttempts

Signed-off-by: SIDDARTHA REDDY <75976672+SIDDARTHAREDDY8@users.noreply.github.com>
@cursor

cursor Bot commented Oct 12, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your trial has ended. Reactivate Greptile to resume code reviews.

@vercel

vercel Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

3 Skipped Deployments
Project Deployment Actions Updated
helicone Skipped Skipped Oct 12, 2026 5:08pm UTC
helicone-bifrost Skipped Skipped Oct 12, 2026 5:08pm UTC
helicone-eu Skipped Skipped Oct 12, 2026 5:08pm UTC

Request Review

@vercel
vercel Bot temporarily deployed to Preview – helicone October 12, 2026 17:08 Inactive
@vercel
vercel Bot temporarily deployed to Preview – helicone-bifrost October 12, 2026 17:08 Inactive
@vercel
vercel Bot temporarily deployed to Preview – helicone-eu October 12, 2026 17:08 Inactive
@SIDDARTHAREDDY8

Copy link
Copy Markdown
Author

Just checking whether there is anything you would like adjusted here — happy to revise.

This branch was previously deployed

3 inactive deployments
Preview – helicone-bifrost — cbb3eb66 Deployed Oct 12, 2026 by vercel[bot]
Preview – helicone-eu — cbb3eb66 Deployed Oct 12, 2026 by vercel[bot]
Preview – helicone — cbb3eb66 Deployed Oct 12, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AttemptBuilder.buildAttempts silently skips invalid model strings (TODO flagged in source) — user typos in fallback lists produce no clear error

1 participant